feat(worktree): isolated rescue worktree, leave-branch cleanup (based on openai#137)#12
Merged
Merged
Conversation
… on openai#137) Adds git-worktree isolation for write-capable rescue (/codex:rescue --worktree), the write-race fix tracked in fork#10 / openai#135. This commit ports the library layer + tests only; companion (--worktree flag) and markdown wiring follow in subsequent PRs. Based on @peterdrier's openai#137, refactored to a "leave-branch" cleanup model. The original capture-then-remove model (snapshot -> apply patch -> force-remove worktree + delete branch) had a deep fail-open class: `git add -A` skips ignored files (.env, dist/), dirty submodules and binary content can't be fully captured, and there is an inherent TOCTOU window between snapshot and remove. Four adversarial review cycles against the straight port found 10 data-loss paths of this shape, each fix closing one edge and the next cycle finding another. Removing the Destroy path removes the entire class by construction. Design (leave-branch): cleanupWorktreeSession NEVER removes the worktree or its branch. keep applies the tracked patch into repoRoot (byte-preserving via `git diff --cached --binary --output`, no JS-string round-trip) and leaves the worktree + branch for the user to inspect and remove manually. discard is a no-op. The cost is one `git worktree remove --force <path> && git branch -D <branch>` per rescue — acceptable for a rare, high-stakes operation whose whole point is write-safety. TOCTOU and the discard-after-failed-keep trap close for free because there is no Destroy path to gate. Files: - plugins/codex/scripts/lib/git.mjs: createWorktree, getWorktreeDiff (gitChecked, throw on failure), applyWorktreePatch (byte-preserving patch transfer). No removeWorktree/deleteWorktreeBranch/hasIgnoredChanges — those belonged to the removed Destroy path. - plugins/codex/scripts/lib/worktree.mjs (NEW): createWorktreeSession / diffWorktreeSession / cleanupWorktreeSession (leave-branch contract). - plugins/codex/scripts/lib/render.mjs: renderWorktreeTaskResult / renderWorktreeCleanupResult — always preservation + manual-remove instructions. - tests/worktree.test.mjs (NEW, 11 tests): session creation, diff capture, and the leave-branch invariant (worktree + branch survive keep tracked-only / ignored-only / mixed tracked+ignored, and discard no-op), byte round-trip, render. The invariant tests replace the old removal-assertion tests and are the proof the fail-open class is closed. Verified: worktree 11/11, full npm test 152 pass / 4 pre-existing (unchanged). Refs fork#10, openai#135, openai#137 (@peterdrier). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
…-closed runCommand Cycle-1 review of PR #12 (Codex + Claude subagent). Two real findings: 1. (Codex) The rendered inspection command was plain `git -C <path> diff`, but rescue changes are STAGED — getWorktreeDiff/applyWorktreePatch run `git add -A`, so `git diff` (unstaged-only) can read empty even when staged tracked/binary/formerly-untracked work exists. A user following the instructions would see "no changes", then force-remove the worktree + branch and irreversibly lose the staged work. Fixed: render a HEAD/base- based diff (`git diff <baseCommit> --binary --submodule=diff`) that includes staged state, plus a `git status --porcelain --ignored -uall` step, and a warning that ignored/submodule work is NOT in the diff. Regression test asserts the inspection command references baseCommit (not a bare `diff`). 2. (Claude) runCommand still normalized a signal-terminated OR spawn-failed child to status:0 (`result.status ?? 0`). A killed git apply (OOM, timeout) or missing git binary (ENOENT) read as success → false-success reporting on worktree capture/apply. Under leave-branch this is no longer data-loss (worktree preserved), but it's a correctness defect with the same root cause. Fixed structurally in process.mjs runCommand: `failed = signal != null || error; status = failed ? 1 : ...`. Regression tests: SIGKILL self-kill and ENOENT both report non-zero status. Codex also flagged "worktree isolation is dead code" — IRRELEVANT to this PR by design: this is the library layer; companion (--worktree wiring) is the next PR. The leave-branch invariant claim (Claude) is confirmed: no production Destroy path exists. Verified: worktree 12/12, process 5/5; full npm test 155 pass / 4 pre-existing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
Owner
Author
🔍 Local review (cycle 1)Reviewed locally (Claude subagent + Codex companion).
Triage cycle 1/3: 2 FIX applied, 1 IRRELEVANT (scoped out), leave-branch claim confirmed. Cycle 2 re-runs on 1465145. |
Owner
Author
🔍 Local review (cycle 2) — clean, shipBoth reviewers: Codex APPROVE (0 findings), Claude CLEAN (all 3 claims verified empirically).
Triage cycle 2/3: 0 FIX. Leave-branch refactor merge-ready. Local mode — merge on user trigger. |
This was referenced Jul 19, 2026
Closed
axisrow
added a commit
that referenced
this pull request
Jul 20, 2026
Security hardening release: leave-branch worktree cleanup (#12) + 4 security fixes (#18 symlink guard, #20 core.symlinks=false neutralize, #22 eliminate info/exclude write, #23 accumulation warn). rescue default --background (#8). test-teardown (#5). All verified with cycle-review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Isolated git-worktree support for write-capable
/codex:rescue --worktree— the write-race fix tracked in fork#10 / openai#135. This PR is the library layer + tests only; companion (--worktreeflag) and markdown wiring follow in subsequent PRs.Attribution
Based on @peterdrier's openai/codex-plugin-cc#137 (
feat: add --worktree flag for isolated write-capable rescue tasks). That PR contributed the worktree-session design, the diff-capture logic, and the test scaffolding. This fork adapted the cleanup model (see below) and the byte-preserving patch transfer.Why this is not a straight port: the leave-branch refactor
openai#137's cleanup is capture-then-remove: snapshot the worktree as a patch → apply it to repoRoot (
keep) → force-remove the worktree + delete the branch. When porting it, four adversarial review cycles (Codex + Claude subagent) found 10 data-loss fail-open paths, all one shape: the Destroy step fires when Capture was incomplete or wrong:git add -Askips git-ignored files (.env,dist/,.output) — paths Codex/rescue writes to routinely — so an ignored-only or mixed worktree produces an empty/incomplete patch, and force-remove destroys the un-captured work.0xff/NUL/binary bytes to U+FFFD and applying the corrupted patch as "success".worktree remove.Each cycle we patched one edge; the next found another. The Capture can never be provably complete under this model, so a
completeboolean gate would just aggregate the same fallible assumptions.Resolution (consensus of a Plan-architect agent + an independent Codex review): remove the Destroy path entirely. Under leave-branch,
cleanupWorktreeSessionNEVER removes the worktree or its branch:git diff --cached --binary --output=<file>— git→file, never through a JS string) and leaves the worktree + branch for the user to inspect and remove manually.The entire fail-open class disappears by construction — there is no Destroy path to gate. TOCTOU and the discard-after-failed-keep trap close for free. The cost is one manual
git worktree remove --force <path> && git branch -D <branch>per rescue, which is the correct ergonomic trade for a rare, high-stakes operation whose whole purpose is write-safety.Files
plugins/codex/scripts/lib/git.mjs—createWorktree,getWorktreeDiff(gitChecked, throw on failure),applyWorktreePatch(byte-preserving patch transfer).plugins/codex/scripts/lib/worktree.mjs(NEW) — session lifecycle with the leave-branch contract.plugins/codex/scripts/lib/render.mjs—renderWorktreeTaskResult/renderWorktreeCleanupResult(always preservation + manual-remove instructions; no destructive command).tests/worktree.test.mjs(NEW, 11 tests) — session creation, diff capture, and the leave-branch invariant (worktree + branch survive keep tracked-only / ignored-only / mixed, and discard no-op), byte round-trip, render. The invariant tests are the proof the fail-open class is closed.Verified
worktree.test.mjs11/11; fullnpm test152 pass / 4 pre-existing (unchanged, unrelated — env-leak on this machine).Refs fork#10, openai#135, openai#137.